Skip to content

fix(ingestion): R1/R2 fences + R7/R9/R23 RPC hardening + R24e stage-FK removal#380

Merged
BigSimmo merged 2 commits into
mainfrom
claude/ingestion-concurrency-rpc-fixes
Jul 8, 2026
Merged

fix(ingestion): R1/R2 fences + R7/R9/R23 RPC hardening + R24e stage-FK removal#380
BigSimmo merged 2 commits into
mainfrom
claude/ingestion-concurrency-rpc-fixes

Conversation

@BigSimmo

@BigSimmo BigSimmo commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Implements the RPC-hardening subset of the phase-3 concurrency work-order (#377), off current main (which now carries the R11 janitor guard + R1 lease heartbeat from the merged #369). Follows docs/ingestion-state-machine.md §8 and docs/supabase-migration-reconciliation.md (expand/contract).

Migration 20260708120000 — NOT applied to live (operator apply step). schema.sql reconciled, drift-manifest.json regenerated via Docker (clean 13s scratch replay).

  • R1/R2 lease fences — complete_ingestion_job + fail_or_retry_ingestion_job gain an optional p_worker_id. A worker reclaimed by a stale takeover matches 0 rows → returns ok:false lease_lost instead of clobbering the reclaimer's newer state (indexed→failed, completed→pending). Backward compatible (p_worker_id defaults null, so today's 4-/8-arg calls still resolve) — the DB migration can ship before the worker redeploy; the worker passes workerId, treats lease_lost as a clean stop, and keeps the isMissingSchemaError fallback for the pre-apply window. Defense-in-depth complement to the heartbeat now on main.
  • R7 — attempt-exhausted retries downgrade to terminal failed instead of a permanently-unclaimable pending that pins the batch as processing forever.
  • R9 — refresh_import_batch_status locks the import_batches row before counting, serializing concurrent last-job refreshes (the lost-update that stuck batches).
  • R23 — cleanup_abandoned_document_index_generations re-asserts the open-job guard immediately before the destructive deletes (previously only in the candidate CTE, before the seven counts).

Verified: eslint ✅ prettier ✅ tsc ✅ · vitest drift-detection + supabase-schema replay guards + ingestion (69 tests) ✅ · drift:manifest regenerated ✅. check:indexing / reindex:health / check:drift (live) and the migration apply are operator steps.

Deliberately excluded (documented in the #377 work-order, with live evidence): R17 open-jobs unique index (drift backlog item #7 owns the ingestion_jobs index reshape; live is clean so it's safe whenever done there); R24e stage-FK (drift backlog item #8 plans to add it, but live has 253 orphans that would fail VALIDATE → evidence says remove — owner decision); deep-memory delete-scoping (would crash — document_sections has unique (document_id, section_index)).

Refs #377 #369 #342
🤖 Generated with Claude Code

@BigSimmo @claude
fix(ingestion): R1/R2 lease fences + R7/R9/R23 RPC hardening (migration)
ebc21d5 
Committed migration 20260708120000 (NOT applied to live — operator apply per
docs/supabase-migration-reconciliation.md), schema.sql reconciled, drift manifest
regenerated (13s scratch replay, clean).

- R1/R2: complete_ingestion_job + fail_or_retry_ingestion_job take an optional
  p_worker_id lease fence. A worker reclaimed by a stale takeover now matches 0
  rows and returns ok:false lease_lost instead of clobbering the reclaimer's
  newer state (indexed->failed, completed->pending). Backward compatible
  (p_worker_id defaults null); the worker passes workerId and treats lease_lost
  as a stop, keeping the isMissingSchemaError fallback for pre-apply. This is the
  defense-in-depth complement to the lease heartbeat already in this PR.
- R7: fail_or_retry downgrades an attempt-exhausted retry to terminal 'failed'
  instead of a permanently-unclaimable 'pending' that pins its batch forever.
- R9: refresh_import_batch_status locks the import_batches row before counting,
  serializing concurrent last-job refreshes (lost-update that stuck batches as
  'processing').
- R23: cleanup_abandoned re-asserts the open-job guard immediately before the
  destructive deletes (it previously lived only in the candidate CTE, before the
  seven count statements).

HELD (evidence in PR): R17 open-jobs unique index (collides with drift backlog
#7, ingestion_jobs index reshape); R24e stage-FK removal (collides with drift
backlog #8, which plans the opposite — live has 253 orphan rows that would fail
VALIDATE); deep-memory delete-scoping (would crash — document_sections
unique(document_id,section_index)).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Jul 8, 2026

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@supabase

supabase Bot commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown

Updates to Preview Branch (claude/ingestion-concurrency-rpc-fixes) ↗︎

Deployments Status Updated
Database ✅ Wed, 08 Jul 2026 06:57:55 UTC
Services ✅ Wed, 08 Jul 2026 06:57:55 UTC
APIs ✅ Wed, 08 Jul 2026 06:57:55 UTC

Tasks are run on every commit but only new migration files are pushed.
Close and reopen this PR if you want to apply changes from existing seed or migration files.

Tasks Status Updated
Configurations ✅ Wed, 08 Jul 2026 06:57:57 UTC
Migrations ✅ Wed, 08 Jul 2026 06:57:59 UTC
Seeding ✅ Wed, 08 Jul 2026 06:58:01 UTC
Edge Functions ✅ Wed, 08 Jul 2026 06:58:01 UTC

View logs for this Workflow Run ↗︎.
Learn more about Supabase for Git ↗︎.

@BigSimmo
BigSimmo enabled auto-merge (squash) July 8, 2026 06:55
@BigSimmo
fix(ingestion): rename hardening migration to 20260708130000 (version…
da9f943 
… collision)

20260708120000 was already taken by 20260708120000_rag_query_misses_retention.sql
on main; two migrations sharing a version fail the schema_migrations pkey on
replay/apply. Content unchanged; schema.sql + drift manifest unaffected (manifest
keys on schema.sql sha, not migration versions).
@BigSimmo
BigSimmo merged commit d013942 into main Jul 8, 2026
5 checks passed
@BigSimmo

BigSimmo commented Jul 8, 2026

Copy link
Copy Markdown
Owner Author

Review — strong LGTM

Reviewed the migration + worker changes for correctness. Well-scoped and follows the migration-reconciliation discipline; only minor nits.

Correctness confirmed

  • No retrieval-RPC seqscan risk. These are language plpgsql, but they operate on small, id-indexed tables (ingestion_jobs, import_batches) — none of the generic-plan-seqscan-over-embeddings trap that applied to the hybrid retrieval RPCs. plpgsql is right here.
  • R1/R2 lease fences — p_worker_id defaults null so today's 4-/8-arg calls still resolve (migration can ship before the worker redeploy); is distinct from is null-safe; on lease_lost it returns before touching documents/siblings, so a reclaimed worker can't clobber the reclaimer's state.
  • R7 — retry gated on attempt_count < max_attempts, consistent with attempts-consumed-at-claim, so exhausted retries go terminal failed instead of an unclaimable pending.
  • R9 — FOR UPDATE on the batch row before counting; lock order (jobs → batch) is consistent across both mutating RPCs, so no new deadlock.
  • R23 — the re-assertion is real and defense-in-depth: the pre-delete re-filter excludes docs with open jobs and every DELETE is generation-scoped (index_generation_id is distinct from d.metadata->>index_generation_id), so even the residual TOCTOU window only removes legitimately-superseded rows.
  • Worker — treats ok:false as a clean stop (no cache invalidation / no demotion) and keeps the isMissingSchemaError fallback for the pre-apply window. Correct.

Findings

  1. [Low] Body/filename mismatch — the PR description says migration 20260708120000, but the file is 20260708130000_ingestion_concurrency_rpc_hardening.sql. An operator following the body could look for the wrong file; worth correcting the description.
  2. [Info] Pre-redeploy residual race — after the migration applies but before the worker passes workerId (null p_worker_id), fail_or_retry can still demote a document a reclaimer just completed. Documented + mitigated by the on-main heartbeat — just confirming it's understood.
  3. [Nit] failOrRetryJob has a redundant double return inside the if (!error) block — harmless.

Nothing blocking; #1 is the only thing I'd change before merge.

🤖 Automated review by Claude Code

@BigSimmo BigSimmo mentioned this pull request Jul 8, 2026
Merged
@BigSimmo BigSimmo changed the title fix(ingestion): R1/R2 lease fences + R7/R9/R23 RPC hardening (migration) fix(ingestion): R1/R2 fences + R7/R9/R23 RPC hardening + R24e stage-FK removal Jul 8, 2026
This was referenced Jul 8, 2026
Closed
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@BigSimmo